Skip to content

fix: remove If-Match header and stop clearing storage on 412#139

Closed
kptdobe wants to merge 1 commit into
mainfrom
fix/no-ifmatch-412
Closed

fix: remove If-Match header and stop clearing storage on 412#139
kptdobe wants to merge 1 commit into
mainfrom
fix/no-ifmatch-412

Conversation

@kptdobe

@kptdobe kptdobe commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

persistence.put sent If-Match: * on every PUT to da-admin. This header tells da-admin "only apply this write if the resource already exists." When the document didn't exist yet (new document, DO cold-start before the first successful write), da-admin returned 412 Precondition Failed.

The 412 handler then:

  1. Called ydoc.storage.deleteAll() — destroying the entire Yjs state, including images not yet flushed to da-admin
  2. Closed all WebSocket connections — forcing all clients to reconnect and reload from a now-empty da-admin

This was the chain that caused inserted images to permanently disappear.

Root cause

If-Match: * has no useful purpose in the collab context:

  • Collab is the authoritative writer for a document once a session is open
  • Whether the resource already exists in da-admin or not, the PUT should always proceed
  • The syncadmin/deleteadmin APIs are the correct mechanism for coordinating external changes

Solution

  • Remove If-Match: * from persistence.put — da-admin now always accepts PUT unconditionally
  • Remove the 412 special-case handler — a 412 (or any other non-2xx) is now treated like any other PUT failure: log the error, surface it via the ydoc error map, and return the current content so the debounced save can retry on the next edit
  • Only 401/403 still trigger session termination (genuine auth failures)

Tests

  • Two new tests assert the correct behavior: If-Match header is absent on PUT; 412 does NOT clear storage or close connections
  • Test persistence update shows error but preserves state on 412 replaces the old "closes all and cleans storage" test
  • Four existing put tests updated: If-Match: * assertions replaced with null assertions

🤖 Generated with Claude Code

@bosschaert

bosschaert commented May 8, 2026

Copy link
Copy Markdown
Contributor

I think the If-Match header was added to fix a specific issue: b33ae65

@karlpauls could you please review?

@bosschaert
bosschaert requested a review from karlpauls May 8, 2026 12:27
@kptdobe kptdobe mentioned this pull request May 11, 2026
Merged
@kptdobe @claude
fix: remove If-Match header and stop clearing storage on 412
c728a3b 
The If-Match: * header on PUT requests caused da-admin to return 412
whenever the document did not yet exist (new document, or first write
after DO cold-start). The 412 handler then called storage.deleteAll()
and closed all WebSocket connections, destroying the entire Yjs state
including images that had not yet been flushed to da-admin.

Remove If-Match: * entirely — da-admin should always accept a PUT from
collab whether the resource exists or not. On any non-2xx response, show
an error on the ydoc but do not touch storage or close connections;
only 401/403 still trigger session termination.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kptdobe
kptdobe force-pushed the fix/no-ifmatch-412 branch from e6e9fe6 to c728a3b Compare May 11, 2026 07:55
@kptdobe

kptdobe commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing — root cause is in da-admin, not da-collab

After deeper analysis, the root cause of the image loss is a bug in da-admin's If-Match: * handling, not in da-collab's use of the header. Removing If-Match: * from da-collab would regress the intentional no-implicit-creation capability introduced by #105.

What actually happens

When da-collab sends PUT with If-Match: *, da-admin correctly validates that the document exists (returning 412 only when current.status === 404). However, it then uses the document's current ETag for the actual R2 write (If-Match: <etag>). If a concurrent write (e.g. during a DO live migration) changes the ETag between the existence check and the write, R2 rejects the PUT with 412.

At that point da-admin has two paths:

  • No client conditional → retry transparently (fetches fresh ETag and tries again)
  • Client sent If-Match → return 412 immediately without retry

The second path conflates If-Match: * (wildcard — "document must exist") with If-Match: "<specific-etag>" (version lock — "document must have this exact version"). A wildcard should behave like the no-conditional path on ETag mismatch because the document still exists; only a specific ETag warrants a non-retriable 412.

The fix

da-admin PR to follow — putObjectWithVersion retries on R2 ETag mismatch when If-Match: * was sent, exactly as it does for internal (no-conditional) writes. If-Match: "<specific-etag>" still returns 412 without retry.

da-collab: no changes needed.

@kptdobe kptdobe closed this May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karlpauls karlpauls Awaiting requested review from karlpauls

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kptdobe @bosschaert